Correct trace name resolution #541

swapster · 2020-03-09T16:58:00Z

Related to #461.

There is a problem with resolution of a trace name for traces without spans with empty refs (when a "root span" is a span with remote parent).

This PR fixes this problem as suggested in a discussion. To detect a "root span" we:

Find all spans that either have no parent reference, or a parent reference that is not found among all trace's spans.
Sort found spans by start time.
Take a first ("the earliest") span and make a trace name from this span's data.

Additionally, there are some unit tests for a different cases.

codecov · 2020-03-09T17:08:00Z

Codecov Report

Merging #541 into master will decrease coverage by 0.02%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #541      +/-   ##
==========================================
- Coverage   92.97%   92.94%   -0.03%     
==========================================
  Files         197      197              
  Lines        4840     4848       +8     
  Branches     1174     1177       +3     
==========================================
+ Hits         4500     4506       +6     
- Misses        300      302       +2     
  Partials       40       40

Impacted Files	Coverage Δ
packages/jaeger-ui/src/model/trace-viewer.tsx	`100.00% <100.00%> (ø)`
...eViewer/TimelineHeaderRow/TimelineViewingLayer.tsx	`88.13% <0.00%> (-3.39%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 68e282d...cd8d04c. Read the comment docs.

1. Find all spans without parent reference or with a parent reference that is not found in the trace 2. Sort all found spans by start time (in case when there are more than one span found) 3. Take first of the found spans and get a trace name Signed-off-by: Valerii Varankin <swapster@yandex-team.ru>

* Test for trace without root span * Test for trace with exactly one root span (with remote ref) * Test for trace with exactly one root span (with no refs) * Test for trace with more than one root span Signed-off-by: Valerii Varankin <swapster@yandex-team.ru>

* File extension changed to .tsx * Path to changed file removed from tsconfig.lint.json * Added type for spans argument in getTraceName() Signed-off-by: Valerii Varankin <swapster@yandex-team.ru>

yurishkuro

thanks for the PR

yurishkuro · 2020-03-09T19:54:09Z

packages/jaeger-ui/src/model/find-trace-name.test.js

@@ -0,0 +1,172 @@
+// Copyright (c) 2018 Uber Technologies, Inc.


yurishkuro · 2020-03-09T19:56:51Z

packages/jaeger-ui/src/model/trace-viewer.tsx

+      const parentIDs = sp.references.filter(r => r.refType === 'CHILD_OF').map(r => r.spanID);
+
+      // no parent from trace
+      return !parentIDs.some(pID => allSpanIDs.includes(pID));


Doesn't allSpanIDs.includes do a linear search? Why not construct allSpanIDs as a dictionary {spanID->span}?

Just didn't think about it. But yeap, dictionary makes sense here.
Changed an array to a dict.

yurishkuro · 2020-03-09T19:58:13Z

packages/jaeger-ui/src/model/trace-viewer.tsx

+      // no parent from trace
+      return !parentIDs.some(pID => allSpanIDs.includes(pID));
+    })
+    .sort((sp1, sp2) => sp1.startTime - sp2.startTime)[0];


I don't think this is correct, as it does not give preference to the true root span (that one that has NO parent). It's only correct in the absence of clock skews.

I missed it.

Added a parents check to a sort() method (also, left untouched comparing by a startTime since, I think, it's better than not to sort multiple "root" spans at all if there are several of them).

Also, changed a couple of tests.

yurishkuro · 2020-03-09T19:59:02Z

packages/jaeger-ui/tsconfig.lint.json

@@ -30,7 +30,6 @@
    "src/api/jaeger.js",
    "src/components/SearchTracePage/SearchResults/ResultItem.markers.js",
    "src/model/order-by.js",
-    "src/model/trace-viewer.js",


should the tsx file be added to some other linter config, or will it be automatically linted?

I will be linted since this config file inherits another config with include option.
Please correct me if I'm wrong.

packages/jaeger-ui/src/model/find-trace-name.test.js

yurishkuro · 2020-03-09T20:01:10Z

packages/jaeger-ui/src/model/find-trace-name.test.js

+  const spansWithNoRoots = [
+    {
+      spanID: firstSpanId,
+      startTime: 1583758690000,


I recommend defining a constant t and using deltas like t+10 to vary the timestamps.

Thanks, fixed.

yurishkuro · 2020-03-09T20:02:12Z

packages/jaeger-ui/src/model/find-trace-name.test.js

+      },
+      references: [
+        {
+          spanID: remoteSpanId,


nit: this looks more like missingSpanId rather than remote.

Yes, its' better, fixed.

Fixed a year in a copyright text Signed-off-by: Valerii Varankin <swapster@yandex-team.ru>

Renamed variable from "remoteSpanId" to "missingSpanId" Signed-off-by: Valerii Varankin <swapster@yandex-team.ru>

Made a constant with base span's timestamp in a tests Signed-off-by: Valerii Varankin <swapster@yandex-team.ru>

Usage of dictionary of all spans instead of id's array to check an inclusion Signed-off-by: Valerii Varankin <swapster@yandex-team.ru>

Correction of a spans sorting to such order: 1. First, sort spans by a presence of a parent 2. Second, sort spans by a start time Signed-off-by: Valerii Varankin <swapster@yandex-team.ru>

* Test for a trace with multiple root spans different by parents * Test for a trace with multiple root spans different by start time Signed-off-by: Valerii Varankin <swapster@yandex-team.ru>

Added a process key with an empty object to all test spans to avoid reading property of undefined Signed-off-by: Valerii Varankin <swapster@yandex-team.ru>

packages/jaeger-ui/src/model/find-trace-name.test.js

packages/jaeger-ui/src/model/trace-viewer.tsx

packages/jaeger-ui/src/model/find-trace-name.test.js

yurishkuro · 2020-03-11T03:20:52Z

packages/jaeger-ui/src/model/trace-viewer.tsx

+      if (!sp.references || !sp.references.length) {
+        return true;
+      }
+      const parentIDs = sp.references.filter(r => r.refType === 'CHILD_OF').map(r => r.spanID);


What if you dropped filters for CHILD_OF? It's possible to have an async trace using FOLLOWS_FROM. I don't think the algorithm is actually sensitive to the type of reference. If anything, I would filter on r.spanContext.traceID being the same as the current span, because a reference to an external trace is definitely not a parent.

Hm. Did I understand correctly, span.references can't contain refs to a children? I mean, if some span listed as a reference, it can be either a parent or a some missing ref, but not a child?
If so, I suggest to change this logic by this way. There is no need to calculate a dict of all trace spans anymore, since some() can check traceID by itself. If there are no situations when spans (which is an argument to this function) contains more data then all .references, this approach looks more correct.

P. S. "I didn't find spanContext in a types, think it should be r.traceID or r.span.traceID :)"

Sorry, I was wrong.

There may be situations when there are references with same traceID but with incorrect (missing) spanID. It this case such span won't define as a root but it can be a root.

Updated a logic and a tests. Now it should work correct.

Co-Authored-By: Yuri Shkuro <yurishkuro@users.noreply.github.com> Signed-off-by: Valerii Varankin <swapster@yandex-team.ru>

….test.js Co-Authored-By: Yuri Shkuro <yurishkuro@users.noreply.github.com> Signed-off-by: Valerii Varankin <swapster@yandex-team.ru>

Co-Authored-By: Yuri Shkuro <yurishkuro@users.noreply.github.com> Signed-off-by: Valerii Varankin <swapster@yandex-team.ru>

Method of parents definition changed to comparing traceIDs of spans Signed-off-by: Valerii Varankin <swapster@yandex-team.ru>

Tests traces data was changed to fit parents definition method by a traceIDs Signed-off-by: Valerii Varankin <swapster@yandex-team.ru>

Added a checking of parent span presence among all trace's spans Signed-off-by: Valerii Varankin <swapster@yandex-team.ru>

Tests traces data was changed according to changed logic Signed-off-by: Valerii Varankin <swapster@yandex-team.ru>

yurishkuro

LGTM !

* Trace name resolution by root span with remote ref 1. Find all spans without parent reference or with a parent reference that is not found in the trace 2. Sort all found spans by start time (in case when there are more than one span found) 3. Take first of the found spans and get a trace name Signed-off-by: Valerii Varankin <swapster@yandex-team.ru> * Unit tests for getTraceName() method * Test for trace without root span * Test for trace with exactly one root span (with remote ref) * Test for trace with exactly one root span (with no refs) * Test for trace with more than one root span Signed-off-by: Valerii Varankin <swapster@yandex-team.ru> * trace-viewer to .tsx * File extension changed to .tsx * Path to changed file removed from tsconfig.lint.json * Added type for spans argument in getTraceName() Signed-off-by: Valerii Varankin <swapster@yandex-team.ru> * Year fix Fixed a year in a copyright text Signed-off-by: Valerii Varankin <swapster@yandex-team.ru> * Variable rename Renamed variable from "remoteSpanId" to "missingSpanId" Signed-off-by: Valerii Varankin <swapster@yandex-team.ru> * Timestamp constant Made a constant with base span's timestamp in a tests Signed-off-by: Valerii Varankin <swapster@yandex-team.ru> * Spans id array to spans dictionary Usage of dictionary of all spans instead of id's array to check an inclusion Signed-off-by: Valerii Varankin <swapster@yandex-team.ru> * Correct sort logic Correction of a spans sorting to such order: 1. First, sort spans by a presence of a parent 2. Second, sort spans by a start time Signed-off-by: Valerii Varankin <swapster@yandex-team.ru> * Tests for a new sorting logic * Test for a trace with multiple root spans different by parents * Test for a trace with multiple root spans different by start time Signed-off-by: Valerii Varankin <swapster@yandex-team.ru> * Empty process object to all test spans Added a process key with an empty object to all test spans to avoid reading property of undefined Signed-off-by: Valerii Varankin <swapster@yandex-team.ru> * Update Copyright text in packages/jaeger-ui/src/model/trace-viewer.tsx Co-Authored-By: Yuri Shkuro <yurishkuro@users.noreply.github.com> Signed-off-by: Valerii Varankin <swapster@yandex-team.ru> * Update Copyright text in packages/jaeger-ui/src/model/find-trace-name.test.js Co-Authored-By: Yuri Shkuro <yurishkuro@users.noreply.github.com> Signed-off-by: Valerii Varankin <swapster@yandex-team.ru> * More detailed comment for root span Co-Authored-By: Yuri Shkuro <yurishkuro@users.noreply.github.com> Signed-off-by: Valerii Varankin <swapster@yandex-team.ru> * Note about a loop Co-Authored-By: Yuri Shkuro <yurishkuro@users.noreply.github.com> Signed-off-by: Valerii Varankin <swapster@yandex-team.ru> * Change of parents definition logic Method of parents definition changed to comparing traceIDs of spans Signed-off-by: Valerii Varankin <swapster@yandex-team.ru> * Tests correction according to changed logic Tests traces data was changed to fit parents definition method by a traceIDs Signed-off-by: Valerii Varankin <swapster@yandex-team.ru> * Checking of parent span presence Added a checking of parent span presence among all trace's spans Signed-off-by: Valerii Varankin <swapster@yandex-team.ru> * Tests correction according to changed logic Tests traces data was changed according to changed logic Signed-off-by: Valerii Varankin <swapster@yandex-team.ru> Co-authored-by: Yuri Shkuro <yurishkuro@users.noreply.github.com> Signed-off-by: vvvprabhakar <vvvprabhakar@gmail.com>

swapster requested review from everett980 and tiffon as code owners March 9, 2020 16:58

swapster added 3 commits March 9, 2020 20:56

trace-viewer to .tsx

e968cdb

* File extension changed to .tsx * Path to changed file removed from tsconfig.lint.json * Added type for spans argument in getTraceName() Signed-off-by: Valerii Varankin <swapster@yandex-team.ru>

swapster force-pushed the correct-trace-name-resolution branch from 79a7e39 to e968cdb Compare March 9, 2020 18:03

yurishkuro reviewed Mar 9, 2020

View reviewed changes

swapster added 7 commits March 10, 2020 12:28

Year fix

5c633c2

Fixed a year in a copyright text Signed-off-by: Valerii Varankin <swapster@yandex-team.ru>

Variable rename

08d8dbb

Renamed variable from "remoteSpanId" to "missingSpanId" Signed-off-by: Valerii Varankin <swapster@yandex-team.ru>

Timestamp constant

5793005

Made a constant with base span's timestamp in a tests Signed-off-by: Valerii Varankin <swapster@yandex-team.ru>

Spans id array to spans dictionary

52e3794

Usage of dictionary of all spans instead of id's array to check an inclusion Signed-off-by: Valerii Varankin <swapster@yandex-team.ru>

Correct sort logic

c914bcf

Correction of a spans sorting to such order: 1. First, sort spans by a presence of a parent 2. Second, sort spans by a start time Signed-off-by: Valerii Varankin <swapster@yandex-team.ru>

Tests for a new sorting logic

a6f67ba

* Test for a trace with multiple root spans different by parents * Test for a trace with multiple root spans different by start time Signed-off-by: Valerii Varankin <swapster@yandex-team.ru>

Empty process object to all test spans

270a762

Added a process key with an empty object to all test spans to avoid reading property of undefined Signed-off-by: Valerii Varankin <swapster@yandex-team.ru>

swapster requested a review from yurishkuro March 10, 2020 10:34

Merge branch 'master' into correct-trace-name-resolution

75552d8

yurishkuro reviewed Mar 11, 2020

View reviewed changes

swapster and others added 4 commits March 11, 2020 11:19

Update Copyright text in packages/jaeger-ui/src/model/trace-viewer.tsx

3944f05

Co-Authored-By: Yuri Shkuro <yurishkuro@users.noreply.github.com> Signed-off-by: Valerii Varankin <swapster@yandex-team.ru>

Update Copyright text in packages/jaeger-ui/src/model/find-trace-name…

f65375f

….test.js Co-Authored-By: Yuri Shkuro <yurishkuro@users.noreply.github.com> Signed-off-by: Valerii Varankin <swapster@yandex-team.ru>

More detailed comment for root span

98b7eb7

Co-Authored-By: Yuri Shkuro <yurishkuro@users.noreply.github.com> Signed-off-by: Valerii Varankin <swapster@yandex-team.ru>

Note about a loop

bfbce5b

Co-Authored-By: Yuri Shkuro <yurishkuro@users.noreply.github.com> Signed-off-by: Valerii Varankin <swapster@yandex-team.ru>

swapster force-pushed the correct-trace-name-resolution branch from 207ec09 to bfbce5b Compare March 11, 2020 08:20

swapster added 2 commits March 11, 2020 12:31

Change of parents definition logic

9aa5b4b

Method of parents definition changed to comparing traceIDs of spans Signed-off-by: Valerii Varankin <swapster@yandex-team.ru>

Tests correction according to changed logic

0c49d43

Tests traces data was changed to fit parents definition method by a traceIDs Signed-off-by: Valerii Varankin <swapster@yandex-team.ru>

swapster requested a review from yurishkuro March 11, 2020 09:57

swapster added 2 commits March 11, 2020 14:19

Checking of parent span presence

8d3eb81

Added a checking of parent span presence among all trace's spans Signed-off-by: Valerii Varankin <swapster@yandex-team.ru>

Tests correction according to changed logic

3b2cbae

Tests traces data was changed according to changed logic Signed-off-by: Valerii Varankin <swapster@yandex-team.ru>

yurishkuro approved these changes Mar 12, 2020

View reviewed changes

Merge branch 'master' into correct-trace-name-resolution

cd8d04c

yurishkuro merged commit a9dce8b into jaegertracing:master Mar 12, 2020

swapster mentioned this pull request Mar 13, 2020

Usage of a helper for trace name when preparing trace data #544

Merged

tmthrgd mentioned this pull request Apr 12, 2021

Fix trace name resolution for spans with remote parents #461

Closed

This was referenced Jul 30, 2021

Sync relevant jaeger-ui PRs grafana/grafana#37404

Closed

Jaeger: Sync getTraceName improvements from upstream grafana/grafana#37408

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Correct trace name resolution #541

Correct trace name resolution #541

swapster commented Mar 9, 2020

codecov bot commented Mar 9, 2020 •

edited

Loading

yurishkuro left a comment

yurishkuro Mar 9, 2020

swapster Mar 10, 2020

yurishkuro Mar 9, 2020

swapster Mar 10, 2020

yurishkuro Mar 9, 2020

swapster Mar 10, 2020

yurishkuro Mar 9, 2020

swapster Mar 10, 2020

yurishkuro Mar 9, 2020

swapster Mar 10, 2020

yurishkuro Mar 9, 2020

swapster Mar 10, 2020

yurishkuro Mar 11, 2020

swapster Mar 11, 2020

swapster Mar 11, 2020

yurishkuro left a comment

		@@ -0,0 +1,172 @@
		// Copyright (c) 2018 Uber Technologies, Inc.

Correct trace name resolution #541

Correct trace name resolution #541

Conversation

swapster commented Mar 9, 2020

codecov bot commented Mar 9, 2020 • edited Loading

Codecov Report

yurishkuro left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yurishkuro left a comment

Choose a reason for hiding this comment

codecov bot commented Mar 9, 2020 •

edited

Loading